Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Support for the {:opaque} attibute on const #2545

Merged
merged 17 commits into from
Aug 25, 2022

Conversation

fabiomadge
Copy link
Collaborator

Fixes #1338.

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@fabiomadge
Copy link
Collaborator Author

module X {
const {:opaque} x := 3
}

const {:opaque} x := 3

function {:opaque} f(i: int): int { i + 1 }

lemma L()
  ensures f(2) == 3
{
    reveal f();
    reveal x;
}

@fabiomadge fabiomadge marked this pull request as ready for review August 25, 2022 18:27
Copy link
Collaborator

@RustanLeino RustanLeino left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! And this will be a good feature to have.


module Module {
module M {
const {:opaque} Five := 5;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The semi-colon at the end of the line is not needed.

Comment on lines 10 to 12
assert M.Five == 5; // error: this is not known here
reveal M.Five;
assert M.Five == 5;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test might not have the effect you think. If you assert something twice, I expect Dafny to complain only about the first assert. This is because Dafny's semantics of assert allows it to implicitly assume the condition after the assert. So, if you remove the reveal, you probably will still get just one verification error.

Instead, I suggest that you create two paths through your method:

if * {
  assert M.Five == 5;
} else {
  reveal M.Five;
  assert M.Five == 5;
}

or

if
case true =>
  assert M.Five == 5;
case true =>
  reveal M.Five;
  assert M.Five == 5;

Twostate-Resolution.dfy(481,11): Error: expression is not allowed to invoke a lemma (reveal_OrdinaryOpaque)
Twostate-Resolution.dfy(481,11): Error: to reveal a function (OrdinaryOpaque), append parentheses
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test file (Twostate-Resolution.dfy) still has comments like (admittedly, a poor error message). Please remove those now, since your error messages are good.

@@ -6575,7 +6575,9 @@ AND Apply(f,h0,bxs) == Apply(f,h0,bxs)
var cf = (ConstantField)f;
if (cf.Rhs != null && RevealedInScope(cf)) {
var etran = new ExpressionTranslator(this, predef, NewOneHeapExpr(f.tok));
sink.AddTopLevelDeclaration(ff.CreateDefinitionAxiom(etran.TrExpr(cf.Rhs)));
if (!Attributes.Contains(cf.Attributes, "opaque")) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Translator has a method IsOpaqueFunction, which checks if a function is opaque. I suggest you define an analogous method for constants and use it here. Alternatively, change the current IsOpaqueFunction to have the name IsOpaque and take any member declaration.

List<MemberDecl> newDecls = new List<MemberDecl>();
foreach (MemberDecl member in c.Members) {
if (member is ConstantField constant) {
if (!Attributes.Contains(constant.Attributes, "opaque")) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comments in Translator.cs about having a method IsOpaque. I guess such a method can be defined in DafnyAst.cs or Resolver.cs instead of in Translator.cs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't add it yet, but could add it to MemberDecl.

Comment on lines 1007 to 1009
Attributes lemma_attrs = new Attributes("auto_generated", new List<Expression>(), null);
lemma_attrs = new Attributes("opaque_reveal", new List<Expression>(), lemma_attrs);
lemma_attrs = new Attributes("verify", new List<Expression>() { new LiteralExpr(c.tok, false) }, lemma_attrs);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect something similar is done for opaque functions. Can the code be shared?

Comment on lines 11106 to 11108
ResolveNameSegment(expr as NameSegment, true, null, revealResolutionContext, true);
} else {
ResolveDotSuffix(expr as ExprDotName, true, null, revealResolutionContext, true);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For expr as NameSegment and expr as ExprDotName, you expect that expr will have those respective types. In other words, you expect that the as will not return null. To indicate this in the program text, I suggest you instead use a cast: (NameSegment)expr and (ExprDotName)expr.

}
MemberSelectExpr callee = (MemberSelectExpr)((ConcreteSyntaxExpression)expr).ResolvedExpression;
if (callee == null) {
} else if ((callee.Member is Lemma or TwoStateLemma && Attributes.Contains(callee.Member.Attributes, "axiom"))) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the outermost pair of parentheses is redundant.

Comment on lines 421 to 422
rewriters.Add(new OpaqueConstRewriter(this.reporter));
rewriters.Add(new OpaqueFunctionRewriter(this.reporter));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest combining these two rewriters into one OpaqueRewriter.

@fabiomadge fabiomadge changed the title feat: const {:opaque} Support for the {:opaque} attibute on const Aug 25, 2022
@fabiomadge fabiomadge changed the title Support for the {:opaque} attibute on const feat: Support for the {:opaque} attibute on const Aug 25, 2022
@fabiomadge fabiomadge enabled auto-merge (squash) August 25, 2022 22:10
@fabiomadge fabiomadge merged commit 1472e40 into dafny-lang:master Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support {:opaque} attribute in constants
2 participants